Repository navigation
fix: show an image count for image-only commits in notifications - #407
udaykiranchenna wants to merge 4 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hbenabdallah
left a comment
hbenabdallah
left a comment
There was a problem hiding this comment.
Looks good. Merged locally onto current production, the new tests pass (NotificationControllerTest, and NotificationIndex.test.tsx / NotificationInbox.test.tsx with bun test).
One thing about the new .tsx tests: CI doesn't run them. test:unit only picks up tests/Unit/*.test.ts, and they aren't listed in tests/draft-images.config.ts, so they only guard the change when run by hand. Adding them to that config would put them in CI. Also, the image-count test matches the exact markup (class="mt-2 rounded-lg border[^"]*"><p class="[^"]*"><svg…), so any styling change to the block will break it. Asserting on the label text inside the bordered block would be less brittle.
…notification-excerpt
Moves NotificationIndex and NotificationInbox to vite-plus/test and lists them in tests/draft-images.config.ts, which SeriesStash.test.ts runs in CI. The image-count test now checks the label text inside the bordered block instead of its exact markup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@hbenabdallah Thanks for the review! Both done:
I also merged current |
…notification-excerpt Resolves the conflicts with dev-null-sh#489, which moved the bordered block into OwnCommitExcerpt and shows the quoted commit under quotes. The quoted commit now loads its image count too, so an image-only one shows "Image" instead of an empty block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged current #489 moved the bordered block into The CI coverage command passes locally (3245 tests, 100%). |
|
@udaykiranchenna, #661 (merged Oct 19, the day of your last update) covers stars on image-only commits, with a different approach: a thumbnail of the first image on the right, "starred your image(s)", and no excerpt box ( 
The other cases this PR handles are still empty on @xHeaven Would it make sense to extend #661's thumbnail to those, rather than the "N images" label, so image-only commits look the same across the inbox? That would mean loading #1078 and #1311 also change how |
Problem
A commit can be images alone, with no text. In the notifications inbox such a commit had nothing to show: a star, approval or rejection drew an empty bordered box, and an image-only reply, quote or mention showed nothing under the sentence. The notification payload didn't say the commit had images, so the page couldn't tell.
Fix
NotificationGroupsQueryloads each notification's commit withwithCount('images'), both for the inbox and for an expanded group (members()).NotificationGroupControllerno longer loads the commit itself, sincemembers()now does.DESIGN.mdmentions it.Notifications only cover approved commits (or your own, for moderation), and a commit only loses its images once it is deleted, so an empty body always comes with at least one image.
Before / After
An image-only reply (top) and a star on an image-only commit (bottom).
Tests
NotificationControllerTest: an image-only commit comes with emptybody_segmentsand itsimages_count, in the inbox and in an expanded group.NotificationIndex.test.tsx: a starred image-only commit renders "Image" or "3 images" inside the bordered block. Both cases fail without the fix.NotificationIndex.test.tsxandNotificationInbox.test.tsxgainimages_count.Lint, PHPStan, type coverage and the full suite (2459 tests, 100% coverage) pass locally.
bun run test:unitonly picks up.tsfiles, so I ran the two.tsxnotification tests withnode --testafter bundling them. In a browser, light and dark: no layout shift, no horizontal overflow, no console errors.